Created nd_backup_schedule module using Ansible Network Resource Modules Approach (DCNE-496) #169

sajagana · 2025-08-20T10:08:49Z

No description provided.

…ule jobs on ND 4.1 and later

… return the specific object

…k Resource Modules Approach

akinross · 2025-08-22T14:47:25Z

plugins/modules/nd_backup_schedule.py

+    state = nd.params.get("state")
+    schedules = nd.request(base_path, method="GET").get("schedules")
+
+    remote_schedule_map = wrap_objects_by_key(schedules)


The assumption here is that you have a single identifier key, is this something that we would like to do?

akinross · 2025-08-22T14:48:38Z

plugins/modules/nd_backup_schedule.py

+
+    result = compare_config_and_remote_objects(schedules, config)
+
+    if state != "query":


this seems redundant because of the nd.exit_json() in line 209

akinross · 2025-08-22T14:49:41Z

plugins/modules/nd_backup_schedule.py

+            nd.after = schedules
+        nd.exit_json()
+
+    nd.before = copy.deepcopy(schedules)


should before and after not both display the same output for query states ?

akinross · 2025-08-22T14:52:07Z

plugins/modules/nd_backup_schedule.py

+
+    remote_schedule_map = wrap_objects_by_key(schedules)
+
+    if state == "query":


Are we supposed to expose query state? If I look at the doc you shared it seems fact modules should handle this, but perhaps I misunderstand it.

akinross · 2025-08-22T14:55:24Z

plugins/modules/nd_backup_schedule.py

+
+    nd.before = copy.deepcopy(schedules)
+
+    result = compare_config_and_remote_objects(schedules, config)


I find the naming of this function a bit weird because the order of your function is reversed of your input to the function.

akinross · 2025-08-22T15:19:11Z

plugins/modules/nd_backup_schedule.py

+        nd.after.append(payload)
+
+
+def get_backup_schedule_time(scheduler_date, scheduler_time):


should this be a generic time handle utils function? likely we will need datetime formatting for other modules

akinross · 2025-08-22T15:21:15Z

plugins/modules/nd_backup_schedule.py

+    if not compare_unordered_list_of_dicts(nd.after, copy.deepcopy(nd.before)):
+        nd.result["changed"] = True


is this needed? cant we tell during a non GET request that is made already that there is a change?

akinross · 2025-08-22T15:22:56Z

plugins/modules/nd_backup_schedule.py

+    nd.exit_json()
+
+
+def post_backup_schedule_config(nd, module, path, config_obj, remote_obj=None, method="POST"):


not sure the function name really makes sense when the method can also be PUT

akinross · 2025-08-22T15:24:52Z

plugins/modules/nd_backup_schedule.py

+
+        if state not in ["deleted", "absent"]:
+            for object in result.get("config_data_create"):
+                post_backup_schedule_config(nd, module, base_path, object)


did you investigate creating a generic request handler, and a module specific payload creator ( I assume that the end part of the function is repeatable for other modules )

akinross · 2025-08-22T15:26:21Z

plugins/modules/nd_backup_schedule.py

+    config = nd.params.get("config")
+    state = nd.params.get("state")
+    schedules = nd.request(base_path, method="GET").get("schedules")
+


Generic overall question, did you consider usage of class structure in python? this way the repeatable conditionals could be done from a single class method, which makes the module perhaps more readable.

akinross · 2025-08-25T07:13:43Z

plugins/module_utils/utils.py

@@ -0,0 +1,67 @@
+# -*- coding: utf-8 -*-


do we want to start to write unit tests for all these newly introduced utils function?

I agree, I think we should be adding unit tests for util functions like these.

samiib · 2025-08-22T12:13:37Z

plugins/module_utils/nd.py

+                if key in sent:
                    try:
                        del sent[key]
                    except KeyError:


You may not need this try/except around this anymore if key is being checked first.

samiib · 2025-08-25T12:05:48Z

plugins/module_utils/nd.py

    def set_to_empty_string_when_none(self, val):
        return val if val is not None else ""
+
+    def get_object_by_nested_key_value(self, path, nested_key_path, value, data_key=None):


Can we add a doc string explanation for this function?

samiib · 2025-08-25T12:07:45Z

plugins/module_utils/utils.py

@@ -0,0 +1,67 @@
+# -*- coding: utf-8 -*-


I agree, I think we should be adding unit tests for util functions like these.

samiib · 2025-08-25T12:17:32Z

plugins/module_utils/utils.py

+    }
+
+
+def compare_unordered_list_of_dicts(list1, list2):


Does this function also take into consideration that lists within the dictionaries could also be ordered different? Do we only need unordered comparison on the top level and not recursive?

samiib · 2025-08-25T12:34:56Z

plugins/modules/nd_backup_schedule.py

+---
+module: nd_backup_schedule
+version_added: "0.5.0"
+short_description: Manages backup schedules on Cisco Nexus Dashboard.


Suggested change

short_description: Manages backup schedules on Cisco Nexus Dashboard.

short_description: Manage backup schedules on Cisco Nexus Dashboard.

samiib · 2025-08-25T12:37:26Z

plugins/modules/nd_backup_schedule.py

+DOCUMENTATION = r"""
+---
+module: nd_backup_schedule
+version_added: "0.5.0"


Is 0.5.0 correct here? Should it be the next version of the collection instead?

samiib · 2025-08-25T13:05:08Z

plugins/modules/nd_backup_schedule.py

+        description:
+        - The frequency at which remote backups are scheduled to occur at specified intervals on selected days.
+        - This parameter is required when creating the backup schedule.
+        type: int


The spec says that frequency is an enum of either 1,7 or 30 days. This is also consistent with the UI.
Should this be changed by adding choices of [1 7 30]?

sajagana added 4 commits August 20, 2025 15:34

[minor_change] Added ND backup schedule module to manage backup sched…

39dd8a5

…ule jobs on ND 4.1 and later

[ignore] Added get_object_by_nested_key_value function to nd.py

955b9b1

[ignore] Changed the get_object_by_nested_key_value function logic to…

5c22c2d

… return the specific object

[ignore] Added backup_type to the nd_backup_schedule module tests

3cab814

sajagana force-pushed the nd_backup_schedule_new branch 2 times, most recently from e7eae7a to d018675 Compare August 20, 2025 12:05

[minor_change] Created nd_backup_schedule module using Ansible Networ…

a6465d2

…k Resource Modules Approach

sajagana force-pushed the nd_backup_schedule_new branch from d018675 to a6465d2 Compare August 20, 2025 12:26

sajagana changed the title ~~Created nd_backup_schedule module using Ansible Network Resource Modules Approach~~ Created nd_backup_schedule module using Ansible Network Resource Modules Approach (DCNE-496) Aug 20, 2025

sajagana changed the base branch from master to codeowners August 20, 2025 14:17

sajagana changed the base branch from codeowners to master August 20, 2025 14:18

sajagana marked this pull request as ready for review August 20, 2025 14:19

sajagana requested review from akinross, anvitha-jain, gmicol, lhercot, samiib and shrsr as code owners August 20, 2025 14:19

akinross reviewed Aug 22, 2025

View reviewed changes

akinross reviewed Aug 25, 2025

View reviewed changes

samiib requested changes Aug 25, 2025

View reviewed changes


		result = compare_config_and_remote_objects(schedules, config)

		if state != "query":


		remote_schedule_map = wrap_objects_by_key(schedules)

		if state == "query":


		nd.before = copy.deepcopy(schedules)

		result = compare_config_and_remote_objects(schedules, config)

		nd.after.append(payload)


		def get_backup_schedule_time(scheduler_date, scheduler_time):

		if not compare_unordered_list_of_dicts(nd.after, copy.deepcopy(nd.before)):
		nd.result["changed"] = True

		nd.exit_json()


		def post_backup_schedule_config(nd, module, path, config_obj, remote_obj=None, method="POST"):

	short_description: Manages backup schedules on Cisco Nexus Dashboard.
	short_description: Manage backup schedules on Cisco Nexus Dashboard.

Created nd_backup_schedule module using Ansible Network Resource Modules Approach (DCNE-496) #169

Are you sure you want to change the base?

Created nd_backup_schedule module using Ansible Network Resource Modules Approach (DCNE-496) #169

Uh oh!

Conversation

sajagana commented Aug 20, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!